Fix browsable API not supporting multipart/form-data correctly #5176

levic · 2017-05-25T10:20:53Z

Autodetect missing boundary parameter for Content-Type header
textarea value normalises EOL chars to \n when multipart/form-data requires \r\n

- Autodetect missing boundary parameter for Content-Type header - textarea value normalises EOL chars to \n when multipart/form-data requires \r\n

tomchristie

Couple of questions here - 1. could you make the reason for that particular regex more clear. 2. What's the intent of re.exec? 3. Could you include before/after screenshots in the PR description?

Looking good!

levic · 2017-05-30T08:30:52Z

Have explained a bit more with comments what the code is doing
Have replaced re.exec(String) with String.match(Regex) (same output but may be a bit clearer)

levic · 2017-05-30T08:32:51Z

Not sure what I can show in a screenshot -- the UI hasn't changed and what happens is often dependent on your application code.

Old behaviour with `multipart/form-data`

If you fill in Content & submit, DRF's API Browser will print the following exception:

HTTP 400 Bad Request
Allow: POST, OPTIONS
Content-Type: application/json
Vary: Accept

{
    "detail": "Multipart form parse error - Invalid boundary in multipart: None"
}

If you Content empty then the request will make it through to your view, but request.data will be empty. Output will depend on how your view handles this.

New behaviour with `multipart/form-data`

When JS can detect a boundary:
- Your view will process the POST request
- The API browser output will make a new GET request; output will be the response from this GET (the same behaviour as other media types)
When JS is unable to detect a boundary (including where content is empty):
- Request is left unmodified by JS
- For empty content your view will be called with empty request.data. Output will depend on how your view handles this.
- For non-empty content, no boundary will be added to the header and django will raise the same exception as before (Multipart form parse error - Invalid boundary in multipart: None). Your view will not be called and the Browser API will show the multipart form parse exception

tomchristie · 2017-05-30T12:07:31Z

rest_framework/static/rest_framework/js/ajax-form.js

+      // We need to add a boundary parameter to the header
+      // We assume the first valid-looking boundary line in the body is correct
+      // regex is from RFC 2046 appendix A
+      var re = /^--([0-9A-Z'()+_,-./:=?]{1,70})[ \f\t\v\u00a0\u1680\u180e\u2000-\u200a\u2028\u2029\u202f\u205f\u3000\ufeff]*$/im;


Can we reduce the whitespace part of this regex to \s* or something similar?

The boundary grouping isn't quite correct against the spec, as it can include " ", it just can't do so at the end.

The only mandatory global parameter for the "multipart" media type is
the boundary parameter, which consists of 1 to 70 characters from a
set of characters known to be very robust through mail gateways, and
NOT ending with white space.

boundary := 0*69<bchars> bcharsnospace

Now handles space in the boundary delimiter correctly (with a more obvious translation from the spec)

Simplified whitespace to \s*? (the non-greedy qualifier because we want the $ to match the current line only and \s includes \n)

Re-tested locally with and without whitespace in the boundary; django handles this correctly too.

tomchristie · 2017-05-30T12:14:22Z

Great work on this! A couple of comments worth addressing there.

…er correctly

levic · 2017-06-15T05:47:45Z

(Comments should be addressed)

tomchristie · 2017-06-16T10:27:22Z

rest_framework/static/rest_framework/js/ajax-form.js

+      // We need to add a boundary parameter to the header
+      // We assume the first valid-looking boundary line in the body is correct
+      // regex is from RFC 2046 appendix A
+      var boundaryCharNoSpace = "[0-9A-Z'()+_,-./:=?";


The leading [ here looks wrong.

tomchristie · 2017-06-16T14:08:40Z

Lovely bit of enhancement - thanks! 😎

Fix browsable API not supporting multipart/form-data correctly

94c37c0

- Autodetect missing boundary parameter for Content-Type header - textarea value normalises EOL chars to \n when multipart/form-data requires \r\n

tomchristie reviewed May 26, 2017

View reviewed changes

Improvements to code clarity

1c44ef2

tomchristie reviewed May 30, 2017

View reviewed changes

tomchristie added the Enhancement label May 30, 2017

tomchristie added this to the 3.6.4 Release milestone May 30, 2017

Fix boundary detection regex not handling spaces inside boundary mark…

6b8d601

…er correctly

levic force-pushed the browsable-api-multipart-form-data branch from 191e418 to 6b8d601 Compare May 31, 2017 03:18

tomchristie requested changes Jun 16, 2017

View reviewed changes

Fix regex typo allowing '[' in boundary string

5cbfcbc

tomchristie approved these changes Jun 16, 2017

View reviewed changes

tomchristie merged commit b069b0d into encode:master Jun 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix browsable API not supporting multipart/form-data correctly #5176

Fix browsable API not supporting multipart/form-data correctly #5176

levic commented May 25, 2017

tomchristie left a comment

levic commented May 30, 2017

levic commented May 30, 2017

tomchristie May 30, 2017

tomchristie May 30, 2017

levic May 31, 2017

tomchristie commented May 30, 2017

levic commented Jun 15, 2017

tomchristie Jun 16, 2017

tomchristie commented Jun 16, 2017 •

edited

Fix browsable API not supporting multipart/form-data correctly #5176

Fix browsable API not supporting multipart/form-data correctly #5176

Conversation

levic commented May 25, 2017

tomchristie left a comment

Choose a reason for hiding this comment

levic commented May 30, 2017

levic commented May 30, 2017

Old behaviour with multipart/form-data

New behaviour with multipart/form-data

tomchristie May 30, 2017

Choose a reason for hiding this comment

tomchristie May 30, 2017

Choose a reason for hiding this comment

levic May 31, 2017

Choose a reason for hiding this comment

tomchristie commented May 30, 2017

levic commented Jun 15, 2017

tomchristie Jun 16, 2017

Choose a reason for hiding this comment

tomchristie commented Jun 16, 2017 • edited

Old behaviour with `multipart/form-data`

New behaviour with `multipart/form-data`

tomchristie commented Jun 16, 2017 •

edited